Skip to content

fix shell check error for case-lib/* - #222

Merged
Bin-QA merged 7 commits into
thesofproject:masterfrom
Bin-QA:shellcheck
May 28, 2020
Merged

fix shell check error for case-lib/*#222
Bin-QA merged 7 commits into
thesofproject:masterfrom
Bin-QA:shellcheck

Conversation

@Bin-QA

@Bin-QA Bin-QA commented May 15, 2020

Copy link
Copy Markdown
Contributor

Update for fix shell check error for case-lib/*
expect case-lib/config.sh file

@Bin-QA
Bin-QA requested review from marc-hb and xiulipan as code owners May 15, 2020 09:29
@Bin-QA

Bin-QA commented May 15, 2020

Copy link
Copy Markdown
Contributor Author

Wait for CI system detect error

@marc-hb marc-hb left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing all these fixes, will make future reviews much easier! See comments below.

Comment thread case-lib/hijack.sh Outdated
Comment thread case-lib/hijack.sh Outdated
Comment thread case-lib/hijack.sh
Comment thread case-lib/hijack.sh Outdated
Comment thread case-lib/lib.sh
Comment thread case-lib/opt.sh Outdated
else
[ "${OPT_VALUE_lst[$i]}" -eq 0 ] && echo -e "\t""Default Value: Off" \
|| echo -e "\t""Default Value: On"
echo -e "\t""Default Value: On"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid echo, use printf

printf '\tDefault Value: '
if [ "${OPT_PARM_lst[$i]}" -eq 1 ]; then
   printf '%s\n' "${OPT_VALUE_lst[$i]}"
...
   printf 'On\n'

Comment thread case-lib/pipeline.sh
if [ $# -lt 1 ]; then
dlogi "Topology file name is not specified, unable to run command: ${BASH_SOURCE[-1]}" && exit 1
dlogi "Topology file name is not specified, unable to run command: $SCRIPT_NAME"
exit 1

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

die()

Comment thread case-lib/pipeline.sh
tplg_path=$(func_lib_get_tplg_path "$1") || {
dloge "No available topology for pipeline export"
exit 1
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

die()

Comment thread case-lib/pipeline.sh
Comment thread case-lib/pipeline.sh Outdated
@marc-hb

marc-hb commented May 15, 2020

Copy link
Copy Markdown
Collaborator

About the [WIP] : if you submit the PR as a "github draft" then no reviewer will be automatically added.
https://github.blog/2019-02-14-introducing-draft-pull-requests/

It's too late now, you can't go back to Draft. Next time.

@paulstelian97
paulstelian97 marked this pull request as draft May 18, 2020 08:07
@paulstelian97

Copy link
Copy Markdown

It's too late now, you can't go back to Draft. Next time.

Not too late apparently. It can always be converted back and review requests re-sent.

@paulstelian97
paulstelian97 removed the request for review from xiulipan May 18, 2020 08:08
Bin-QA added 6 commits May 19, 2020 10:57
fix shell check error and cleanup some useless code

Signed-off-by: Wu, BinX <binx.wu@intel.com>
1. remove _func_logcmd_add_timestamp function
direct to use 'alais' for the dlog* command
2. fix shell check error
3. remove some useless code

Signed-off-by: Wu, BinX <binx.wu@intel.com>
1. refine option for sof-tplgreader format
2. fix shell check error and remove some useless code

Signed-off-by: Wu, BinX <binx.wu@intel.com>
fix shell check error and remove some useless code

Signed-off-by: Wu, BinX <binx.wu@intel.com>
fix shell check error and remove some useless code

Signed-off-by: Wu, BinX <binx.wu@intel.com>
use readarray instead of IFS
readarray make code more simple and clean
It also fix shell check error

This change refer
hijack.sh: exit
lib.sh: func_lib_disable_pulseaudio
pipeline.sh: func_pipeline_export
@Bin-QA

Bin-QA commented May 19, 2020

Copy link
Copy Markdown
Contributor Author

@marc-hb

  1. die() function to fix I want to add into other PR,
    it also refer the problem we have different exit code
    so here die() just focus on dloge + exit 1?
  2. echo replace with print just for the opt.sh or other file

Already verify on CI system

@Bin-QA
Bin-QA marked this pull request as ready for review May 19, 2020 05:44
@marc-hb

marc-hb commented May 19, 2020

Copy link
Copy Markdown
Collaborator
  1. I keep losing track of where die() is available versus not. If it's not available here yet then forget it for now.

  2. For very simple strings with no control characters echo is IMHO not the best but acceptable. For anything more advanced like for instance control characters please use printf.

@Bin-QA

Bin-QA commented May 20, 2020

Copy link
Copy Markdown
Contributor Author
  1. For very simple strings with no control characters echo is IMHO not the best but acceptable. For anything more advanced like for instance control characters please use printf.

@marc-hb I already try with printf
but I find, if I want use this style string "description need two\nlines" will not output
like my design, so it need to parse the string, how to deal those string for output

@paulstelian97

Copy link
Copy Markdown

but I find, if I want use this style string "description need two\nlines" will not output
like my design, so it need to parse the string, how to deal those string for output

Use single quotes on that, otherwise the shell will eat the \. Or do double escaping.

@marc-hb

marc-hb commented May 20, 2020

Copy link
Copy Markdown
Collaborator

Exactly: always single quote the format string:

printf 'format: \t%s\nstring: \t%d' "$arg1" "$arg2"
a='pri\nt_this'
echo -e "value:\t$a"
printf 'val:\t%s\n' "$a"

echo command with '-e' option will hide the control char
for the output string replace with printf '%s' to instead of it

Signed-off-by: Wu, BinX <binx.wu@intel.com>
@Bin-QA Bin-QA changed the title [WIP]fix shell check error for case-lib/* fix shell check error for case-lib/* May 21, 2020
@Bin-QA

Bin-QA commented May 25, 2020

Copy link
Copy Markdown
Contributor Author

@marc-hb Can you help me to review this patch, or you think what I need to update

@marc-hb
marc-hb self-requested a review May 28, 2020 04:23

@marc-hb marc-hb left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marc-hb Can you help me to review this patch, or you think what I need to update

Sorry for the delay, this is a large PR and I don't really understand the changes unrelated to shellcheck. Next time please submit several smaller PRs, one at a time thanks!

@Bin-QA
Bin-QA merged commit b4fe945 into thesofproject:master May 28, 2020
@Bin-QA
Bin-QA deleted the shellcheck branch May 28, 2020 04:54
@marc-hb

marc-hb commented Jul 1, 2020

Copy link
Copy Markdown
Collaborator

Next time please submit several smaller PRs, one at a time thanks!

It's not only impossible to review changes that large, it's also impossible to test them in a reasonable time. See for instance regression #266

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants